Skip to content

Conversation

@wujingyue
Copy link
Collaborator

Do you still need it?

Do you still need it?
@wujingyue
Copy link
Collaborator Author

!test

@wujingyue wujingyue requested a review from samnordmann January 3, 2026 17:26
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 3, 2026

Greptile Summary

Removed the disable_skip mechanism that allowed bypassing test skipping through the MULTIDEVICE_DISABLE_SKIP environment variable. This cleanup removes unnecessary complexity from the test framework.

  • Removed disable_skip member variable from MultiDeviceTest class
  • Simplified skip logic in SetUp() and Communication test to directly check communicator availability
  • Cleaned up unused imports (<mutex> and testing::Contains)
  • All references to disable_skip have been completely removed from the codebase

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The changes cleanly remove a deprecated testing mechanism without affecting test logic. All references are removed completely, no dangling dependencies remain, and the simplified skip logic is correct and more maintainable.
  • No files require special attention

Important Files Changed

Filename Overview
tests/cpp/multidevice.cpp Removed disable_skip field initialization and unused mutex include; simplified SetUp() method
tests/cpp/multidevice.h Removed disable_skip member variable from MultiDeviceTest class
tests/cpp/test_multidevice_pipeline.cpp Simplified backend availability check by removing disable_skip condition
tests/cpp/test_multidevice_sharding.cpp Removed unused testing::Contains include and added trailing newline

Sequence Diagram

sequenceDiagram
    participant Env as Environment Variable
    participant Test as MultiDeviceTest
    participant Comm as Communicator
    participant GTest as Google Test Framework

    Note over Env,Test: Before PR: disable_skip mechanism
    Env->>Test: MULTIDEVICE_DISABLE_SKIP env var
    Test->>Test: Constructor: disable_skip = getNvFuserEnv()
    Test->>Test: SetUp() called
    alt disable_skip is false
        Test->>Comm: is_available()?
        Comm-->>Test: false
        Test->>GTest: GTEST_SKIP()
    else disable_skip is true
        Test->>Comm: is_available()
        Comm-->>Test: false
        Note over Test: Test runs anyway (skip disabled)
    end

    Note over Env,Test: After PR: simplified skipping
    Test->>Test: SetUp() called
    Test->>Comm: is_available()?
    alt communicator not available
        Comm-->>Test: false
        Test->>GTest: GTEST_SKIP()
    else communicator available
        Comm-->>Test: true
        Note over Test: Test proceeds normally
    end
Loading

@github-actions
Copy link

github-actions bot commented Jan 3, 2026

Auto-merge Status

✅ Internal CI is finished
✅ No failed checks
✅ PR is mergeable
ℹ️ PR mergeable_state: unstable

Description

  • Remove disable_skip environment variable and associated logic from multi-device tests

  • Simplify conditional checks by removing disable_skip conditions

  • Clean up unused includes and member variables

  • Minor formatting improvements and removal of unused testing imports

Changes walkthrough

Relevant files
Enhancement
multidevice.cpp
Remove disable_skip logic from MultiDeviceTest                     

tests/cpp/multidevice.cpp

  • Remove mutex header include
  • Remove disable_skip environment variable initialization
  • Remove random seed comment
  • Simplify SetUp() condition by removing disable_skip check
  • +1/-4     
    test_multidevice_pipeline.cpp
    Simplify pipeline test skip condition                                       

    tests/cpp/test_multidevice_pipeline.cpp

  • Simplify backend availability check by removing disable_skip condition
  • +1/-1     
    multidevice.h
    Remove disable_skip member variable                                           

    tests/cpp/multidevice.h

    • Remove disable_skip member variable declaration
    +0/-1     
    Formatting
    test_multidevice_sharding.cpp
    Clean up unused imports and formatting                                     

    tests/cpp/test_multidevice_sharding.cpp

  • Remove unused testing::Contains import
  • Add blank line at end of file
  • +1/-1     

    PR Reviewer Guide

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review
    Logic Simplification

    The SetUp() method logic was simplified from if (!disable_skip && !communicator_->is_available()) to if (!communicator_->is_available()). This is correct as the disable_skip functionality is being removed entirely.

    if (!communicator_->is_available()) {
      GTEST_SKIP() << "This test needs an available communicator.";
    }
    Consistent Simplification

    The test case also had its disable_skip check removed, changing from if (!disable_skip && !communicator_->isBackendAvailable(backend)) to if (!communicator_->isBackendAvailable(backend)). This maintains consistency with the overall removal of disable_skip functionality.

    if (!communicator_->isBackendAvailable(backend)) {
      GTEST_SKIP() << "Backend not available";
    }

    @samnordmann
    Copy link
    Collaborator

    Do you still need it?

    No, it can be removed! Thanks

    @wujingyue
    Copy link
    Collaborator Author

    No, it can be removed! Thanks

    Cool! Can you approve the PR then? @samnordmann

    @wujingyue wujingyue added the enable-auto-merge Auto-merge a PR when: 1) PR mergeable 2) Internal CI complete 3) No failures label Jan 5, 2026
    @github-actions
    Copy link

    github-actions bot commented Jan 5, 2026

    ⚠️ Auto-merge failed. Please review and merge manually.

    Workflow run: https://github.com/NVIDIA/Fuser/actions/runs/20725407476

    cc @xwang233

    @github-actions github-actions bot merged commit 8459aa8 into main Jan 6, 2026
    62 of 63 checks passed
    @github-actions github-actions bot removed the enable-auto-merge Auto-merge a PR when: 1) PR mergeable 2) Internal CI complete 3) No failures label Jan 6, 2026
    @github-actions github-actions bot deleted the wjy/skip branch January 6, 2026 13:47
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants